Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

add extra header file to export #13795

Merged
merged 10 commits into from
Jan 9, 2019
Merged

add extra header file to export #13795

merged 10 commits into from
Jan 9, 2019

Conversation

apeforest
Copy link
Contributor

Description

Export this header file for Horovod to throw and catch exception elegantly in MXNet.
Horovod PR: horovod/horovod#542

yuxihu
yuxihu reviewed Jan 8, 2019
View reviewed changes
Copy link
Member

@yuxihu yuxihu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Left a nit comment.

plugin/opencv/cv_api.cc Outdated
@@ -26,9 +26,9 @@
#include <dmlc/base.h>
#include <mxnet/base.h>
#include <mxnet/ndarray.h>
#include <mxnet/c_api_common.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QQ: do we have guideline regarding the order of putting include files? Shall we follow alphabetic order of the filename? If so, please move this include to the right place here and all other places in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't find a guideline for order. I was following the order of full path header followed by relative path header

eric-haibin-lin
eric-haibin-lin reviewed Jan 8, 2019
View reviewed changes
include/mxnet/c_api_common.h Outdated
#ifndef MXNET_C_API_C_API_COMMON_H_
#define MXNET_C_API_C_API_COMMON_H_
#ifndef MXNET_C_API_COMMON_H_
#define MXNET_C_API_COMMON_H_

#include <dmlc/base.h>
#include <dmlc/logging.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need all these functions in the public header files, including MXAPIThreadLocalEntry, CopyAttr, etc?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind pointing out how horovod will use these APIs?

Copy link
Contributor Author

@apeforest apeforest Jan 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eric-haibin-lin I agree MXAPIThreadLocalEntry, CopyAtt are not needed in error handling, nor should we make them visible externally. I don't know the historical reason of adding them to this header file in the first place. My change was following the rationale of making least change to existing structure.

I could extract the error handling functions from c_api_common.h into c_api_error.h and only export c_api_error.h to include. Does that sound like a better approach to you?

szha
szha reviewed Jan 8, 2019
View reviewed changes
Copy link
Member

@szha szha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm concerned about what gets exposed publicly due to this change. I think it's necessary to list the APIs that is needed for horovod integration (and discussing briefly what API such use cases require), and then expose only the necessary subset.

@apeforest
Copy link
Contributor Author

@eric-haibin-lin @szha The PR that requires this change is ctcyang/horovod#19. The reason we need this header is that we are using check_call in Horovod to check the return status here. We need a way to catch the exception thrown from MXNet C API like this. However, the API_BEGIN() and API_END() are defined in c_api_common.h which is the one I need to export.

@eric-haibin-lin
Copy link
Member

eric-haibin-lin commented Jan 8, 2019 •
edited
Loading

I agree that MXNet should expose c apis necessary for error handling. Exposing API_BEGIN/API_END/API_END_HANDLE_ERROR sounds reasonable. We probably should prepend "MX_" in these macro. WDYT @apeforest @szha

@szha
Copy link
Member

szha commented Jan 8, 2019

Let's limit the exposure to only what's required.

@apeforest
Copy link
Contributor Author

@eric-haibin-lin @szha Per your suggestion, I have stripped out the error handling APIs into a separate header file and export that one only. The remaining code in MXNet modules remain unchanged. Please review this change again at your earliest convenience. Thanks a lot!

yuxihu
yuxihu reviewed Jan 8, 2019
View reviewed changes
Copy link
Member

@yuxihu yuxihu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. just a nit comment

src/c_api/c_api_common.h Outdated Show resolved Hide resolved
yuxihu
yuxihu approved these changes Jan 8, 2019
View reviewed changes
Copy link
Member

@yuxihu yuxihu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

szha
szha reviewed Jan 8, 2019
View reviewed changes
include/mxnet/c_api_error.h Outdated
#include <dmlc/base.h>
#include <dmlc/logging.h>
#include <mxnet/c_api.h>
#include <mxnet/base.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are all the headers needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch. removed unnecessary headers

@apeforest apeforest changed the title add extra header file to include add extra header file to export Jan 9, 2019
@eric-haibin-lin eric-haibin-lin merged commit 44c21bd into apache:master Jan 9, 2019
yuxihu pushed a commit to yuxihu/incubator-mxnet that referenced this pull request Jan 9, 2019
@apeforest @yuxihu
Add extra header file to export for error checking (apache#13795)
1590762 
* add extra header file to include

* fix sanity check

* fix sanity

* move c_api_common.h to include folder

* fix build error

* keep c_api_common.h internal

* strip out error handling API into a separate header

* consolidate comment into one paragraph per review

* remove unnecessary include
@yuxihu yuxihu mentioned this pull request Jan 9, 2019
Merged
apeforest added a commit to apeforest/incubator-mxnet that referenced this pull request Jan 11, 2019
@apeforest
Add extra header file to export for error checking (apache#13795)
ffca79e 
* add extra header file to include

* fix sanity check

* fix sanity

* move c_api_common.h to include folder

* fix build error

* keep c_api_common.h internal

* strip out error handling API into a separate header

* consolidate comment into one paragraph per review

* remove unnecessary include
yuxihu pushed a commit to yuxihu/incubator-mxnet that referenced this pull request Jan 14, 2019
@apeforest @yuxihu
Add extra header file to export for error checking (apache#13795)
d02cabd 
* add extra header file to include

* fix sanity check

* fix sanity

* move c_api_common.h to include folder

* fix build error

* keep c_api_common.h internal

* strip out error handling API into a separate header

* consolidate comment into one paragraph per review

* remove unnecessary include
eric-haibin-lin pushed a commit that referenced this pull request Jan 16, 2019
@apeforest @eric-haibin-lin
[v1.4.x] export exception handling APIs in MXNet (#13845)
bf15539 
* Get the correct include path in pip package (#13452)

* add find_include_path API

* address reviewer comment

* change return type from list to string

* add unit test

* address reviewer comment

* address reviewer comment

* address reviewer comment

* address reviewer comment

* fix include path problem in pip package

* add comment

* fix lint error

* address reviewer comment

* address reviewer comment

* Add extra header file to export for error checking (#13795)

* add extra header file to include

* fix sanity check

* fix sanity

* move c_api_common.h to include folder

* fix build error

* keep c_api_common.h internal

* strip out error handling API into a separate header

* consolidate comment into one paragraph per review

* remove unnecessary include
lanking520 pushed a commit to lanking520/incubator-mxnet that referenced this pull request Feb 18, 2019
@apeforest @lanking520
[v1.4.x] export exception handling APIs in MXNet (apache#13845)
2d02f36 
* Get the correct include path in pip package (apache#13452)

* add find_include_path API

* address reviewer comment

* change return type from list to string

* add unit test

* address reviewer comment

* address reviewer comment

* address reviewer comment

* address reviewer comment

* fix include path problem in pip package

* add comment

* fix lint error

* address reviewer comment

* address reviewer comment

* Add extra header file to export for error checking (apache#13795)

* add extra header file to include

* fix sanity check

* fix sanity

* move c_api_common.h to include folder

* fix build error

* keep c_api_common.h internal

* strip out error handling API into a separate header

* consolidate comment into one paragraph per review

* remove unnecessary include
haohuanw pushed a commit to haohuanw/incubator-mxnet that referenced this pull request Jun 23, 2019
@apeforest
Add extra header file to export for error checking (apache#13795)
0148a80 
* add extra header file to include

* fix sanity check

* fix sanity

* move c_api_common.h to include folder

* fix build error

* keep c_api_common.h internal

* strip out error handling API into a separate header

* consolidate comment into one paragraph per review

* remove unnecessary include
@leezu leezu mentioned this pull request Sep 3, 2020
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@eric-haibin-lin eric-haibin-lin eric-haibin-lin approved these changes

@yuxihu yuxihu yuxihu approved these changes

@szha szha szha approved these changes

@anirudh2290 anirudh2290 Awaiting requested review from anirudh2290

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@apeforest @eric-haibin-lin @szha @yuxihu